In doctesting, ignore two ld warnings from recent OS X: #36337
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have seen these warnings on two separate machines, but it would be nice if someone else saw them, too. |
|
Do you have details of commands where you see these warnings? There are several Sage packages where one sees multiple |
Here are a few examples: (and a number more in that file) and (and a number more in that file). Summary: I also see these messages in |
|
They all seem to come from doctests involving |
|
These come from Xcode 15 linker - which caused havoc elsewhere, I presume you also get lots of these in install.log |
|
Can you add a comment on this in the source code, and in the (long) commit message - that it's from Xcode 15 "ld"? |
|
Here's a new commit and I also edited the description at the top of this page. |
dimpase
approved these changes
Sep 28, 2023
|
It's probably can be fixed by Cython people (not too easily, I suppose). Maybe Apple will fix this in their ld, who knows. It should also be possible We're finally switching to Cython 3, #36110, perhaps it's fixed there? |
|
Can you check, by saving the following into # distutils: language = c++
from libcpp.vector cimport vector
cdef vector[int] * v = new vector[int](4)and running that you still see a warning? (Want to make sure it's a small reproducer) |
I see this: |
|
No warning here, hmm. What |
|
But, wait, this is probably not the whole building of an extension function. One should get a .dylib file, or .so file. |
can you attach here this log file? |
|
indeed, here in the log of Sage's probably |
I ran |
Here it is. This is from an Intel Mac. |
|
Possibly related (although they say that particular bug has been fixed): https://developer.apple.com/forums/thread/733317 |
|
See also https://stackoverflow.com/questions/76661812/xcode-15-beta-duplicate-lc-rpath-are-deprecated. Someone suggested adding |
|
As far as duplicate OTOH, in |
|
Can we do something like to eliminate the duplicates? (I'm not sure where this would go.) |
|
Trying to eliminate duplicates is too dangerous because order matters. |
|
Instead we should try to see where the duplicates are introduced |
|
With #36364, the full test-suite for me now shows The first two of those are known (#35646) and the last two are the "duplicate libraries" warnings. No "duplicate -rpath" warnings. |
|
I'm removing the "positive review" tag. Ignoring these warnings shouldn't hurt, so please reinstate if you want, but if we can solve the problem without that, wouldn't it be better? |
|
potentially, "duplicate libraries" might be needed in some cases. So I don't really know what this is all about. Is Apple |
|
They are just warnings, not errors. The post at https://stackoverflow.com/a/77190575/1401572 suggests adding fi
if [ -n "$SAGE_LOCAL" ]; then
- LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
+ LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,-ld_classic,$SAGE_LOCAL/lib $LDFLAGS"
if [ "$UNAME" = "Linux" ]; then
LDFLAGS="-Wl,-rpath-link,$SAGE_LOCAL/lib $LDFLAGS"
fimakes tests pass for cython.py and connectivity.pyx. Is this a good thing to do? If so, only do this for DARWIN, presumably. Only for Xcode 15? |
|
We could do something like this, but I don't know if it's a good idea: @@ -276,6 +275,14 @@ export UNAME=`uname | sed 's/CYGWIN.*/CYGWIN/' `
# Mac OS X-specific setup
if [ "$UNAME" = "Darwin" ]; then
export MACOSX_VERSION=`uname -r | awk -F. '{print $1}'`
+ # Command-line tools version:
+ XCLT_VERSION=`pkgutil --pkg-info=com.apple.pkg.CLTools_Executables | awk '/version: / { print $NF }' | cut -d. -f-1`
+ # If this didn't produce an integer, set to 0.
+ case $XCLT_VERSION in
+ ''|*[!0-9]*) export XCLT_VERSION=0 ;; # bad
+ *) : ;; # good
+ esac
+ export XCLT_VERSION
# Work around problems on recent OS X crashing with an error message
# "... may have been in progress in another thread when fork() was called"
# when objective-C functions are called after fork(). See Issue #25921.
@@ -373,7 +380,11 @@ if [ -n "$PYTHONHOME" ]; then
fi
if [ -n "$SAGE_LOCAL" ]; then
- LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
+ if [ "$UNAME" = "Darwin" ] && [ "$XCLT_VERSION" -ge 15 ]; then
+ LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-ld_classic,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
+ else
+ LDFLAGS="-L$SAGE_LOCAL/lib -Wl,-rpath,$SAGE_LOCAL/lib $LDFLAGS"
+ fi
if [ "$UNAME" = "Linux" ]; then
LDFLAGS="-Wl,-rpath-link,$SAGE_LOCAL/lib $LDFLAGS"
fi |
|
ld_classic is not there forever, I suppose. So using it only postpones a proper fix. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 1, 2023
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> Do not run sage-env more than once. <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> In the previous version of sage-env, in the lines ``` SAGE_ENV_VERSION="6:$SAGE_LOCAL:$SAGE_VENV:$SAGE_SRC" if [ "$SAGE_ENV_SOURCED" = "$SAGE_ENV_VERSION" ]; then # Already sourced, nothing to do. return 0 fi export SAGE_ENV_SOURCED="$SAGE_ENV_VERSION" ``` the test would fail the first time, as it should, but when the rest of the file was executed, SAGE_SRC would get changed. That meant that the test would fail the next time and so the file would get executed again. We move the last line to the end of the file, and also change it to explicitly use `SAGE_LOCAL`, `SAGE_VENV`, and `SAGE_SRC` so that it uses the up-to-date versions. This should fix part of the problem in sagemath#36337: it should prevent duplicate `-rpath` entries which were arising because sage-env was getting executed twice. <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [X] The title is concise, informative, and self-explanatory. - [X] The description explains in detail what this PR is about. - [X] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36364 Reported by: John H. Palmieri Reviewer(s): Dima Pasechnik, John H. Palmieri, Matthias Köppe
|
I don't know where the "duplicate libraries" are coming from. We could
|
|
Likely they are coming in from pkg-config via the linker directives at the top of cython files and sage.env.cython_aliases |
|
Removing the duplicates might be a bad idea — does order matter? — but in this case it fixes the doctest error in |
|
pari might come with cysignals |
vbraun
pushed a commit
that referenced
this pull request
Oct 8, 2023
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> Do not run sage-env more than once. <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> In the previous version of sage-env, in the lines ``` SAGE_ENV_VERSION="6:$SAGE_LOCAL:$SAGE_VENV:$SAGE_SRC" if [ "$SAGE_ENV_SOURCED" = "$SAGE_ENV_VERSION" ]; then # Already sourced, nothing to do. return 0 fi export SAGE_ENV_SOURCED="$SAGE_ENV_VERSION" ``` the test would fail the first time, as it should, but when the rest of the file was executed, SAGE_SRC would get changed. That meant that the test would fail the next time and so the file would get executed again. We move the last line to the end of the file, and also change it to explicitly use `SAGE_LOCAL`, `SAGE_VENV`, and `SAGE_SRC` so that it uses the up-to-date versions. This should fix part of the problem in #36337: it should prevent duplicate `-rpath` entries which were arising because sage-env was getting executed twice. <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [X] The title is concise, informative, and self-explanatory. - [X] The description explains in detail what this PR is about. - [X] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #36364 Reported by: John H. Palmieri Reviewer(s): Dima Pasechnik, John H. Palmieri, Matthias Köppe
ld: warning: duplicate -rpath '/path/to/SAGE_ROOT/local/lib' ignored ld: warning: ignoring duplicate libraries: '-lflint', '-lgmp', '-lmpfr'
mkoeppe
force-pushed
the
ignore-new-ld-warnings
branch
from
2c9a092 to
8f5ee78
Compare
|
rebased |
|
I'm also getting |
|
Documentation preview for this PR (built with commit 8f5ee78; changes) is ready! 🎉 |
I haven't seen these on any machine in |
This was referenced Oct 30, 2023
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 6, 2023
…s (OS X) and set LDFLAGS accordingly
Identify the version of command-line tools in use (OS X), and use the
version to set LDFLAGS.
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
In sage-env, save the version of command-line tools being used in
XCLT_VERSION. If it's too recent, we set LDFLAGS to solve some problems:
- Fixes sagemath#36337 (doctest failures)
- Fixes sagemath#36342 (scipy building,
replacing sagemath#36523).
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [X] The title is concise, informative, and self-explanatory.
- [X] The description explains in detail what this PR is about.
- [X] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36599
Reported by: John H. Palmieri
Reviewer(s): Matthias Köppe
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 7, 2023
…s (OS X) and set LDFLAGS accordingly
Identify the version of command-line tools in use (OS X), and use the
version to set LDFLAGS.
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
In sage-env, save the version of command-line tools being used in
XCLT_VERSION. If it's too recent, we set LDFLAGS to solve some problems:
- Fixes sagemath#36337 (doctest failures)
- Fixes sagemath#36342 (scipy building,
replacing sagemath#36523).
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [X] The title is concise, informative, and self-explanatory.
- [X] The description explains in detail what this PR is about.
- [X] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36599
Reported by: John H. Palmieri
Reviewer(s): Matthias Köppe
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
in doctesting, ignore two warnings coming from ld in new OS X/command-line tools/Xcode 15
As of Xcode 15, I see warnings of this sort arising in doctesting:
These all seem to arise from doctests involving
cython(...).These warnings cause doctests to fail, so we ignore them, as we ignore similar warning messages in src/doctest/parsing.py.
📝 Checklist
⌛ Dependencies